-
Notifications
You must be signed in to change notification settings - Fork 421
turn cluster resolver middleware into a mux that can recognize cluster names in path mappings #3628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test all |
…r names in path mappings On-behalf-of: @SAP [email protected]
6bcb624
to
c767a4d
Compare
/test all |
/test all |
/retest |
/retest |
Summary
This PR implements an extension to the routing capabilities of the front-proxy. Previously it could only perform prefix matches (and those were mostly manually implemented), like sending everything beginning with
/services/
to a virtual-workspace endpoint.With this PR, the configured path in the front-proxy's mapping is treated a a path pattern like
http.ServerMux
expects it, i.e. placeholders like{cluster}
can be used.The use of these placeholders is limited since only core kcp code can access and make use of them. For now, there is a single special placeholder called
{cluster}
which can be used to provide a cluster context especially to virtual workspace endpoints (i.e. make the front-proxy understand that/services/myservice/clusters/abc123/apis/
refers to clusterabc123
). This cluster context is then important when the per-workspace authentication feature is used.I could see this being extended to help in per-shard routing for virtual workspaces, which is currently still an unsolved TODO: now more of the codebase uses a standard
http.ServerMux
, so it forms a good basis for extensions in the future.However, the whole topic of routing in kcp is IMHO still quite confusing. There are special routes like
/clusters/
, there are multiple places throughout the codebase that use differnt methods of parsing URLs. I would at some point like to reorganize and clean it up a bit.Code-wise, this PR replaces the ClusterResolver's custom string parsing to instead use a mux. This allows it to set the cluster in the request context, like we already do in the
/clusters/
-handler. Note that all configured mappings are added twice to the mux in order to be compatible with the old prefix-matching (configuring a path mapping/foo
must match the literal/foo
and/foo/bar
, so two mux routes are necessary).The proxy.HttpHandler was thinned out massively by simply relying on the already resolved data in the request context, rather than doing an index lookup and string parsing all over again. The trick to make this work was to not accidentally lose the query string parameters, which the ClusterResolver middleware up until now did when constructing the sharded target URL.
The mapping types were moved into a standalone package to solve an import loop.
What Type of PR Is This?
/kind feature
Release Notes